Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add traffic permissions integration tests. #19008

Merged
merged 2 commits into from
Oct 6, 2023
Merged

Add traffic permissions integration tests. #19008

merged 2 commits into from
Oct 6, 2023

Conversation

erichaberkorn
Copy link
Contributor

@erichaberkorn erichaberkorn commented Sep 26, 2023

Description

Add integration tests for TCP traffic permissions.

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@erichaberkorn erichaberkorn added pr/no-changelog PR does not need a corresponding .changelog entry pr/no-backport labels Sep 26, 2023
@erichaberkorn erichaberkorn force-pushed the tp-tests branch 8 times, most recently from 5f591b7 to eb17a1e Compare September 28, 2023 13:33
Comment on lines +138 to +145
var ctx context.Context
rtestClient, ok := client.(*Client)
if ok {
ctx = rtestClient.Context(t)
} else {
ctx = testutil.TestContext(t)
rtestClient = NewClient(client)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't particularly like this, but it's hard to do much better without making a breaking change that impacts tons and tons of tests.

client2EchoSuccess: true,
},
// TODO I don't like this behavior.
"allow one protocol doesnt impact the other protocol": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ishustava It feels weird to me that configuring a traffic permission with default allow only swaps to default deny for a single port rather than all port. Should we change this behavior to swap all ports to default deny?

@erichaberkorn erichaberkorn marked this pull request as ready for review September 28, 2023 17:10
@erichaberkorn erichaberkorn requested a review from a team as a code owner September 28, 2023 17:10
@vercel
Copy link

vercel bot commented Oct 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
consul ⬜️ Ignored (Inspect) Visit Preview Oct 4, 2023 4:02pm
consul-ui-staging ⬜️ Ignored (Inspect) Oct 4, 2023 4:02pm

// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

package tproxy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be trafficpermissions?

Copy link
Member

@jmurret jmurret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM. Can you add the comment in code that I mentioned? Once it is merged, I will reference it in the ticket that I create.

USER root

RUN microdnf install -y iptables sudo nc \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a note for something like:

On Mac M1s when TProxy is enabled, consul-dataplane that are spawned from this image (only used in consul-container integration tests) will terminate with the below error. It is related to tproxy-startup.sh calling iptables SDK which then calls the underly iptables. We are investigating how this works on M1s with consul-envoy images which do not have this problem. For the time being tproxy tests on Mac M1s will fail locally but pass in CI.

Error setting up traffic redirection rules: failed to run command: /sbin/iptables -t nat -N CONSUL_PROXY_INBOUND, err: exit status 1, output: iptables: Failed to initialize nft: Protocol not supported

@erichaberkorn erichaberkorn force-pushed the tp-tests branch 2 times, most recently from acaa084 to 53cf850 Compare October 6, 2023 14:58
@erichaberkorn erichaberkorn merged commit ad3aab1 into main Oct 6, 2023
84 checks passed
@erichaberkorn erichaberkorn deleted the tp-tests branch October 6, 2023 16:06
jmurret pushed a commit that referenced this pull request Oct 12, 2023
Add traffic permissions integration tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/no-backport pr/no-changelog PR does not need a corresponding .changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants